Skip to content

overlay.d & tests: Add alternatives migration and test and remove iptables-legacy #3253

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Aug 11, 2025
Merged

overlay.d & tests: Add alternatives migration and test and remove iptables-legacy #3253

merged 3 commits into from
Aug 11, 2025

Conversation

travier
Copy link
Member

@travier travier commented Nov 13, 2024 •
edited by PeaceRebel
Loading

overlay.d: Add empty statoverride config files

overlay.d & tests: Add alternatives migration and test

  • Add an overlay with the migration logic for alternatives
  • Add a test for the migration script

This should make sure that the system is setup properly and that the
migration script will do the right thing on older systems.

See: coreos/fedora-coreos-tracker#1818
See: coreos/fedora-coreos-tracker#677
See: https://docs.fedoraproject.org/en-US/fedora-coreos/alternatives/

  • Remove iptables-legacy starting with Fedora 43

@travier travier mentioned this pull request Nov 14, 2024
Open
HuijingHei
HuijingHei reviewed Nov 15, 2024
View reviewed changes
HuijingHei
HuijingHei reviewed Nov 15, 2024
View reviewed changes
@travier travier force-pushed the testing-devel-alternatives-migration branch from 484d98b to 2ba6b2a Compare November 15, 2024 12:04
aaradhak
aaradhak reviewed Nov 15, 2024
View reviewed changes
@travier travier force-pushed the testing-devel-alternatives-migration branch 2 times, most recently from 086b0fa to 273dbde Compare November 18, 2024 16:18
@travier travier changed the title tests: Add initial alternatives test overlay.d & tests: Add alternatives migration and test Nov 18, 2024
@travier
Copy link
Member Author

travier commented Nov 18, 2024

I've now added the migration script to this PR and a test for it. Not fully tested yet.

HuijingHei
HuijingHei previously approved these changes Dec 5, 2024
View reviewed changes
Copy link
Member

@HuijingHei HuijingHei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM.

@travier travier force-pushed the testing-devel-alternatives-migration branch from 273dbde to 3cfd61b Compare May 14, 2025 09:08
@PeaceRebel PeaceRebel force-pushed the testing-devel-alternatives-migration branch from 3cfd61b to 6c31ebb Compare June 5, 2025 14:24
@PeaceRebel
Copy link
Contributor

PeaceRebel commented Jun 5, 2025 •
edited
Loading

This test was failing after rpm-ostree update including coreos/rpm-ostree#5368. Updated the path to /usr/bin instead of /usr/sbin

jlebon
jlebon reviewed Jun 5, 2025
View reviewed changes
@PeaceRebel PeaceRebel force-pushed the testing-devel-alternatives-migration branch from 6c31ebb to 193cb37 Compare June 6, 2025 09:31
@PeaceRebel PeaceRebel requested review from jlebon and HuijingHei June 6, 2025 09:44
@HuijingHei
Copy link
Member

/lgtm

c4rt0
c4rt0 reviewed Jun 10, 2025
View reviewed changes
Copy link
Member

@c4rt0 c4rt0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the first time I come across the use of alternatives for iptables, regardless...

/lgtm

dustymabe
dustymabe reviewed Jun 10, 2025
View reviewed changes
dustymabe
dustymabe reviewed Jun 10, 2025
View reviewed changes
@PeaceRebel PeaceRebel force-pushed the testing-devel-alternatives-migration branch 2 times, most recently from d21d435 to 3f4040d Compare June 13, 2025 13:48
@PeaceRebel PeaceRebel requested review from dustymabe and c4rt0 June 13, 2025 13:49
dustymabe
dustymabe reviewed Jun 13, 2025
View reviewed changes
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good. A few comments (mostly clarifying questions).

I really don't feel comfortable merging this without a review from @travier though. He should be back next week I think.

@PeaceRebel PeaceRebel force-pushed the testing-devel-alternatives-migration branch from 3f4040d to 5034859 Compare June 16, 2025 12:09
travier
travier commented Jul 11, 2025
View reviewed changes
Copy link
Member Author

@travier travier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add the changes from #3573 here as well? Thanks

@PeaceRebel PeaceRebel force-pushed the testing-devel-alternatives-migration branch from b3880ed to 177b6aa Compare July 11, 2025 13:06
@PeaceRebel PeaceRebel changed the title overlay.d & tests: Add alternatives migration and test overlay.d & tests: Add alternatives migration and test and remove iptables-legacy Jul 11, 2025
@PeaceRebel
Copy link
Contributor

Can you add the changes from #3573 here as well? Thanks

Added it here and closed the other PR.

@dustymabe
Copy link
Member

Let's squash the last two commits together.

Also there is some post script we should clean up. Let's just leave the changes all in fedora-coreos-base and add a new conditional_include in there?

Suggestion:

diff --git a/manifests/fedora-coreos-base.yaml b/manifests/fedora-coreos-base.yaml
index 05b2927a..98e7b24b 100644
--- a/manifests/fedora-coreos-base.yaml
+++ b/manifests/fedora-coreos-base.yaml
@@ -27,6 +27,28 @@ ostree-layers:
   - overlay/25azure-udev-rules
   - overlay/30lvmdevices
 
+conditional-include:
+  - if: releasever < 43
+    include:
+      packages:
+        # iptables-legacy was in <43 but excluded from 43+
+        # https://github.com/coreos/fedora-coreos-tracker/issues/1818
+        - iptables-legacy
+      postprocess:
+        # Default to iptables-nft. Otherwise, legacy wins. We can drop this once/if we
+        # remove iptables-legacy. This is needed because alternatives don't work
+        # https://github.com/coreos/fedora-coreos-tracker/issues/677
+        # https://github.com/coreos/fedora-coreos-tracker/issues/676
+        - |
+          #!/usr/bin/bash
+          set -eux -o pipefail
+          ln -sf /usr/sbin/ip6tables-nft         /etc/alternatives/ip6tables
+          ln -sf /usr/sbin/ip6tables-nft-restore /etc/alternatives/ip6tables-restore
+          ln -sf /usr/sbin/ip6tables-nft-save    /etc/alternatives/ip6tables-save
+          ln -sf /usr/sbin/iptables-nft          /etc/alternatives/iptables
+          ln -sf /usr/sbin/iptables-nft-restore  /etc/alternatives/iptables-restore
+          ln -sf /usr/sbin/iptables-nft-save     /etc/alternatives/iptables-save
+
 # Be minimal
 recommends: false
 
@@ -71,20 +93,6 @@ postprocess:
     set -eux -o pipefail
     systemctl mask dnsmasq.service
 
-  # Default to iptables-nft. Otherwise, legacy wins. We can drop this once/if we
-  # remove iptables-legacy. This is needed because alternatives don't work
-  # https://github.com/coreos/fedora-coreos-tracker/issues/677
-  # https://github.com/coreos/fedora-coreos-tracker/issues/676
-  - |
-    #!/usr/bin/bash
-    set -eux -o pipefail
-    ln -sf /usr/sbin/ip6tables-nft         /etc/alternatives/ip6tables
-    ln -sf /usr/sbin/ip6tables-nft-restore /etc/alternatives/ip6tables-restore
-    ln -sf /usr/sbin/ip6tables-nft-save    /etc/alternatives/ip6tables-save
-    ln -sf /usr/sbin/iptables-nft          /etc/alternatives/iptables
-    ln -sf /usr/sbin/iptables-nft-restore  /etc/alternatives/iptables-restore
-    ln -sf /usr/sbin/iptables-nft-save     /etc/alternatives/iptables-save
-
   # sudo prefers its config files to be mode 440, and some security scanners
   # complain if /etc/sudoers.d files are world-readable.
   # https://bugzilla.redhat.com/show_bug.cgi?id=1981979
@@ -164,9 +172,6 @@ packages:
   - console-login-helper-messages-motdgen
   # i18n
   - kbd
-  # In F35+ need `iptables-legacy` package
-  # See https://github.com/coreos/fedora-coreos-tracker/issues/676#issuecomment-928028451
-  - iptables-legacy
   # NIC firmware we've traditionally shipped but then were split out of linux-firmware in Fedora
   - qed-firmware # https://github.com/coreos/fedora-coreos-tracker/issues/1746
   # Include udev rules for NVMe backed Azure Instances

@PeaceRebel
Copy link
Contributor

Let's just leave the changes all in fedora-coreos-base and add a new conditional_include in there?

Including the alternates overlay changes too? @dustymabe

@dustymabe
Copy link
Member

Let's just leave the changes all in fedora-coreos-base and add a new conditional_include in there?

Including the alternates overlay changes too? @dustymabe

No sorry.. Just the last commit where we are moving the iptables-legacy package (and also the postprocess for setting nft as the default, which won't be needed once we remove the legacy package).

@PeaceRebel PeaceRebel force-pushed the testing-devel-alternatives-migration branch from a9e89b5 to 15a7ba7 Compare July 11, 2025 14:24
@PeaceRebel
Copy link
Contributor

No sorry.. Just the last commit where we are moving the iptables-legacy package (and also the postprocess for setting nft as the default, which won't be needed once we remove the legacy package).

Ok. Updated.

@dustymabe
Copy link
Member

This LGTM, but now the alternatives test (that we are also adding in this PR) can't do the full test because there is no iptables-legacy to set it back to so we can test the migration.

Not sure what to do about that.

Maybe we'll just have to settle for some manual testing?

@PeaceRebel
Copy link
Contributor

This LGTM, but now the alternatives test (that we are also adding in this PR) can't do the full test because there is no iptables-legacy to set it back to so we can test the migration.

Not sure what to do about that.

Maybe we'll just have to settle for some manual testing?

We can probably do the same verification as we do in the extended upgrade test. I guess the upgrade test needs to be updated as well to do the verification if release version is >= 43

@travier
Copy link
Member Author

travier commented Jul 11, 2025

This LGTM, but now the alternatives test (that we are also adding in this PR) can't do the full test because there is no iptables-legacy to set it back to so we can test the migration.

Not sure what to do about that.

Maybe we'll just have to settle for some manual testing?

We can overlay the package and apply live in the test as well.

@PeaceRebel PeaceRebel force-pushed the testing-devel-alternatives-migration branch from 15a7ba7 to 47c677f Compare July 21, 2025 08:58
@PeaceRebel
Copy link
Contributor

Modified the alternatives test to skip on version before 43 and overlay iptables-legacy.

travier
travier commented Jul 23, 2025
View reviewed changes
travier
travier commented Jul 23, 2025
View reviewed changes
@PeaceRebel PeaceRebel force-pushed the testing-devel-alternatives-migration branch from 47c677f to 2c605de Compare July 30, 2025 18:02
travier
travier commented Jul 31, 2025
View reviewed changes
Copy link
Member Author

@travier travier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this should be good for F43 now. Thanks Bipin!

@travier travier requested a review from dustymabe July 31, 2025 08:31
@travier
Copy link
Member Author

travier commented Jul 31, 2025

Requesting an (hopefully) final review from @dustymabe :)

@PeaceRebel
Copy link
Contributor

PeaceRebel commented Aug 7, 2025 •
edited
Loading

@dustymabe, dropping a reminder.
I see the test is failing. I'll take a look tomorrow.

travier and others added 3 commits August 8, 2025 15:26
@travier @PeaceRebel
overlay.d & tests: Add alternatives migration and test
9274d1c 
- Add an overlay with the migration logic for alternatives
- Add a test for the migration script

This should make sure that the system is setup properly and that the
migration script will do the right thing on older systems.

See: coreos/fedora-coreos-tracker#1818
See: coreos/fedora-coreos-tracker#677
See: https://docs.fedoraproject.org/en-US/fedora-coreos/alternatives/

co-authored by: Bipin B Narayan <[email protected]>
@PeaceRebel
Add verification for alternatives migration to extended upgrades test
ebb668d
@PeaceRebel
Remove iptables-legacy starting with Fedora 43 and related tests
4f83d04 
ref: coreos/fedora-coreos-tracker#1818
@PeaceRebel PeaceRebel force-pushed the testing-devel-alternatives-migration branch from 2c605de to 4f83d04 Compare August 8, 2025 09:58
@PeaceRebel
Copy link
Contributor

@dustymabe, dropping a reminder. I see the test is failing. I'll take a look tomorrow.

Fixed it. @dustymabe , can you take a final look?

dustymabe
dustymabe approved these changes Aug 8, 2025
View reviewed changes
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dustymabe dustymabe merged commit 65f9b7d into coreos:testing-devel Aug 11, 2025
5 checks passed
@dustymabe dustymabe mentioned this pull request Aug 11, 2025
Open
@travier travier deleted the testing-devel-alternatives-migration branch September 4, 2025 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PeaceRebel PeaceRebel PeaceRebel left review comments

@c4rt0 c4rt0 c4rt0 left review comments

@aaradhak aaradhak aaradhak left review comments

@dustymabe dustymabe dustymabe approved these changes

@HuijingHei HuijingHei HuijingHei left review comments

@jlebon jlebon Awaiting requested review from jlebon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@travier @PeaceRebel @HuijingHei @dustymabe @jlebon @c4rt0 @aaradhak